Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add grouped memory & cpu columns #1338

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

80avin
Copy link

@80avin 80avin commented Nov 25, 2023

Fixes #301 , #920 , #1154

Add GCPU (Grouped CPU) and GMEM (Grouped Memory) columns which show sum of cpu/memory for the current process & its children.

Open Questions

  • Are keyboard shortcuts to sort these columns acceptable ?
    • Logically, they mirror existing shortcuts of M (memory) to X and P (CPU) to W. I didn't want to choose Q that's why chose 2nd column of keys.
  • Are names shown ( GCPU, GMEM ) acceptable ? Could've used keywords like tree, children to derive new names ( like TCPU, CPUT, or maybe something else. )
  • Tested only on Linux ( Linux 6.5.0-13-generic - Kubuntu 23.10 )

Screenshot

image

@BenBE BenBE added the new feature Completely new feature label Nov 26, 2023
Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you done some performance testing how much this additional column influences the overall performance?

Also it's likely desirable to skip calculating these aggregates, if none of the aggregate columns is used. See the various column flags that are in place for other optional information.

Comment on lines +1109 to +1113
if (parentPid == pid) {
looping = false;

break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to set logging=false; if it's not used beyond this loop anyway.

Also, have a look at Brent's algorithm for loop detection.

Comment on lines +1115 to +1119
if (parentProc == NULL) {
// TODO: or assert ?
looping = false;
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (parentProc == NULL) {
// TODO: or assert ?
looping = false;
break;
}
if (!parentProc) {
break;
}

An assert may trigger accidently here, if the parent of a process is not captured in the process scan (e.g. just started)

@@ -146,9 +146,15 @@ typedef struct Process_ {
/* CPU usage during last cycle (in percent) */
float percent_cpu;

/* Sum of percent_mem for self and children */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Sum of percent_mem for self and children */
/* Sum of percent_cpu for self and children */

@@ -55,10 +55,16 @@ static void ProcessTable_prepareEntries(Table* super) {
Table_prepareEntries(super);
}

static void ProcessTable_aggregate(ProcessTable* this) {
Hashtable_foreach(this->super.table, Process_resetGroupFields, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to include this inside the call to ProcessTable_goThroughEntries, as this avoids one iteration over the hash map (which is not the fastest).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular feedback might take time.

Specifically, I don't want to write anything in platform-specific files as the idea or the implementation isn't platform-specific at all. But I also agree with you on the unnecessary performance cost. In this dilemma, I want to explore my options and find something better, which may take time, maybe in weeks.

The rest of the feedback, including using PROCESS_FLAG_* like flags, is already done.

Comment on lines +47 to +48
PERCENT_CPU_GROUP = 130,
PERCENT_MEM_GROUP = 131,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global fields should be defined in the enum definition before platform-specific ones.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only concern was not to affect existing enum values. Now, I've chosen to make these 127, 128 and had to do AUTOGROUP_ID = 129 , AUTOGROUP_NICE = 130 , CCGROUP = 131

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any place in htop codebase mentioning that global fields should be before platform specific ones. Personally I think htop has no specific rules for assigning field IDs and the current allocation is messy. Ideally the fields IDs should be stable across htop releases so the configuration files can refer the fields by numbers. In addition, we should reserve ranges of field IDs for specific purposes so that, e.g. dynamic fields would not use them causing collisions.

I would cite MediaWiki namespace IDs as a good example of how IDs can be allocated are how IDs are reserved for future uses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a matter of numeric values; just a code ordering. The macro including the platform flags should be the last line in that definition.

Regarding actual allocations: Old versions of htop used to write the field IDs into the config files. With more recent versions this has been changed to use name representations instead, thus the only thing that needs to be kept for BC is with the columns available with old htop versions that still stored the numeric IDs.

But yes, the current assignment is kinda messy, but nothing too easy to fix over night.

[PERCENT_NORM_CPU] = { .name = "PERCENT_NORM_CPU", .title = "NCPU%", .description = "Normalized percentage of the CPU time the process used in the last sampling (normalized by cpu count)", .flags = 0, .defaultSortDesc = true, .autoWidth = true, },
[PERCENT_MEM] = { .name = "PERCENT_MEM", .title = "MEM% ", .description = "Percentage of the memory the process is using, based on resident memory size", .flags = 0, .defaultSortDesc = true, },
[PERCENT_MEM] = { .name = "PERCENT_MEM", .title = "MEM% ", .description = "Percentage of the memory the process and its children are using, based on resident memory size", .flags = 0, .defaultSortDesc = true, },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this description updated?

@Explorer09
Copy link
Contributor

I don't like this.

  1. The GCPU% and GMEM% are bad names. (The "G" can be confused with something else like GPU and graphics memory)
  2. I think a more ideal solution is not to introduce new columns for such things, but to adjust the CPU% and MEM% displays in Tree View so that they display aggregated data when the tree is collapsed.
  3. The sorting in Tree View by CPU% or MEM% might need to be adjusted, too.

@BenBE
Copy link
Member

BenBE commented Nov 26, 2023

I don't like this.

  1. The GCPU% and GMEM% are bad names. (The "G" can be confused with something else like GPU and graphics memory)

Names can be changed. And I'm not quite happy with them either …

  1. I think a more ideal solution is not to introduce new columns for such things, but to adjust the CPU% and MEM% displays in Tree View so that they display aggregated data when the tree is collapsed.

That's mostly a matter of taste, as you can argue both ways. Either you display always the true values and have aggregate columns to add things up, or you have a mixed column, where some values are aggregates and you end up having to mark, which values are aggregates and which are not.

I personally prefer the solution using a separate column, as it does not loose you any information, but to the contrary actually tells you, how much of the aggregate value is from child processes.

  1. The sorting in Tree View by CPU% or MEM% might need to be adjusted, too.

Can you elaborate on this item?

@Explorer09
Copy link
Contributor

@BenBE
The idea of aggregated values of child processes works better when Tree View is in effect, otherwise you will end up seeing duplicated numbers of the parent processes listed first, and their child processes listed further down.
When the CPU% numbers of all processes don't add up to (number of CPU cores × 100%), users would get confused more. That's one of the reasons I don't like a column of aggregated numbers.

The sorting in Tree View by CPU% or MEM% might need to be adjusted, too.

Can you elaborate on this item?

This is my personal opinion. I think the advantage of having a tree view is to have a quick idea which processes are part of a group and which process groups form a session. It's not just about tracking the parents of the processes. So I think there are rooms to improve the Tree View usability, especially with the goal of managing processes in groups and sessions.

When it comes to sorting, I mean the CPU% and MEM% in a tree branch should be ordered by the aggregated value of that branch. For one parent process, the children with the most CPU usage (in total, including all the descendant processes) would be listed first, followed by other children/branches. The similar applies to the MEM usage sorting.

@80avin
Copy link
Author

80avin commented Nov 26, 2023

@Explorer09 I agree with most of what you said.

I don't like this.

  1. The GCPU% and GMEM% are bad names.

I am still thinking about the right name and am not convinced with what current ones. I've mentioned same in PR body also, and will try to find better ones. If using non-alphabetical ASCII characters ( like %CPU┼ , %CPU± , %CPU┬ , %CPU╦ , %CPU» , %CPU* etc.) is fine, then we can have more combinations to try & explore. Not good examples, but I will give it more time.

  1. I think a more ideal solution is not to introduce new columns for such things but to adjust the CPU% and MEM% displays in Tree View so that they display aggregated data when the tree is collapsed.

Actually, I thought in this direction originally. My requirement was simple, to see the aggregate CPU/MEM%, irrespective of whether it is shown by a new setting or a new column. But then I wanted to see how many resources a tree is using vs. the parent & children nodes side-by-side. This could be achieved only by adding a new column type. We can also have more aggregate columns which aren't derived from current ones, like child process count, etc.
However, I still see these as opinions and will accept whatever the majority votes for.

  1. The sorting in Tree View by CPU% or MEM% might need to be adjusted, too.

I also noticed the same. I'm thinking of making a separate PR for that as it will impact the existing columns also.

@80avin
Copy link
Author

80avin commented Nov 26, 2023

Have you done some performance testing how much this additional column influences the overall performance?

A quick and dirty implementation shows that it takes <10 ms on my machine, running ~200tasks at the moment.
I don't think this is good and I'll try to optimize while refactoring how/where the aggregate is calculated.

@Explorer09
Copy link
Contributor

  1. The GCPU% and GMEM% are bad names.

I am still thinking about the right name and am not convinced with what current ones. I've mentioned same in PR body also, and will try to find better ones. If using non-alphabetical ASCII characters ( like %CPU┼ , %CPU± , %CPU┬ , %CPU╦ , %CPU» , %CPU* etc.) is fine, then we can have more combinations to try & explore. Not good examples, but I will give it more time.

Using Unicode is fine in htop, but we would need ASCII fallback as not every user would run htop with UTF-8 mode enabled.

  1. I think a more ideal solution is not to introduce new columns for such things but to adjust the CPU% and MEM% displays in Tree View so that they display aggregated data when the tree is collapsed.

Actually, I thought in this direction originally. My requirement was simple, to see the aggregate CPU/MEM%, irrespective of whether it is shown by a new setting or a new column. But then I wanted to see how many resources a tree is using vs. the parent & children nodes side-by-side. This could be achieved only by adding a new column type. We can also have more aggregate columns which aren't derived from current ones, like child process count, etc. However, I still see these as opinions and will accept whatever the majority votes for.

I knew your idea, but the problem is that there could be a lot of columns introduced just for aggregated stats after we pull in your patch as the first example.

I have a better vision for this: if the aggregated data for a process group or session is important and worth displaying separately from the parent process node, I would introduce separate rows for displaying the aggregated info. Such rows would be colored differently from the processes and threads (for a better visual distinction). As these row do not refer to individual processes or threads:

  • The PID would be a placeholder like *.
  • The "Command" field or summary would say something like [Process group 12345, 5 processes] or [Session 23455, 10 processes]
  • I think these rows for aggregated data would be provided for distinct process groups and sessions only. If you want aggregated data for multiple process groups under a single parent (e.g. a bash shell running several pipes of commands simultaneously), the user would need to add up the stats themselves or close the tree under that bash process.
  • These new rows for aggregated data may be turned off (i.e. hidden) in settings.

How is that?

@BenBE
Copy link
Member

BenBE commented Nov 27, 2023

  1. The GCPU% and GMEM% are bad names.

I am still thinking about the right name and am not convinced with what current ones. I've mentioned same in PR body also, and will try to find better ones. If using non-alphabetical ASCII characters ( like %CPU┼ , %CPU± , %CPU┬ , %CPU╦ , %CPU» , %CPU* etc.) is fine, then we can have more combinations to try & explore. Not good examples, but I will give it more time.

Using Unicode is fine in htop, but we would need ASCII fallback as not every user would run htop with UTF-8 mode enabled.

To be precise: Enabling Unicode support is an optional compile-time feature, thus columns should be able to be displayed with ASCII alone.

  1. I think a more ideal solution is not to introduce new columns for such things but to adjust the CPU% and MEM% displays in Tree View so that they display aggregated data when the tree is collapsed.

Actually, I thought in this direction originally. My requirement was simple, to see the aggregate CPU/MEM%, irrespective of whether it is shown by a new setting or a new column. But then I wanted to see how many resources a tree is using vs. the parent & children nodes side-by-side. This could be achieved only by adding a new column type. We can also have more aggregate columns which aren't derived from current ones, like child process count, etc. However, I still see these as opinions and will accept whatever the majority votes for.

I knew your idea, but the problem is that there could be a lot of columns introduced just for aggregated stats after we pull in your patch as the first example.

ACK. That's also why I'm not really fired-up about this yet. I see both sides and my initial review was from a technical PoV only.

I have a better vision for this: if the aggregated data for a process group or session is important and worth displaying separately from the parent process node, I would introduce separate rows for displaying the aggregated info. Such rows would be colored differently from the processes and threads (for a better visual distinction). As these row do not refer to individual processes or threads:

Not really a great option either, as this causes vertical screen estate to be used up for this.

An idea to explore may be implementing "multi-mode" columns, similar to the meter display mode, where the same column could be switched to display different values (in the configuration).

  • The PID would be a placeholder like *.
  • The "Command" field or summary would say something like [Process group 12345, 5 processes] or [Session 23455, 10 processes]
  • I think these rows for aggregated data would be provided for distinct process groups and sessions only. If you want aggregated data for multiple process groups under a single parent (e.g. a bash shell running several pipes of commands simultaneously), the user would need to add up the stats themselves or close the tree under that bash process.
  • These new rows for aggregated data may be turned off (i.e. hidden) in settings.

How is that?

That looks like #303 on first glance …

@80avin
Copy link
Author

80avin commented Nov 27, 2023

I have a better vision for this: if the aggregated data for a process group or session is important and worth displaying separately from the parent process node, I would introduce separate rows for displaying the aggregated info. Such rows would be colored differently from the processes and threads (for a better visual distinction).

I think the idea needs to be refined more to be convincing. Not sure if it is only me but when I try to evaluate this idea based on the value it adds relative to the development effort, I don't find it much pleasing.
Like, if we want to show aggregate values like number of child processes running for a tree, it won't fit into any existing column.
Correct me if you find I'm missing anything.

An idea to explore may be implementing "multi-mode" columns

"multi-mode" columns also sound like a great idea. If we're having that, then we can also allow the user to add duplicate columns (same columns with different modes) to his screen.
But another perspective on the multi-mode column is that

  1. They will only reduce some clutter from Screen settings, as "Available Columns" will have one entry for process CPU & tree CPU. Otherwise, they'll be working the same as current PR from the user's perspective.
    Is this difference enough of value addition that we implement multi-mode feature for columns ?
  2. Different modes will still require different column names, so this "multi-mode" is just a way to pack multiple column definitions into one in the Settings only.

@Explorer09
Copy link
Contributor

I think the idea needs to be refined more to be convincing. Not sure if it is only me but when I try to evaluate this idea based on the value it adds relative to the development effort, I don't find it much pleasing.

Well, the development effort is not something I would evaluate as htop is an open source project without any definite deadline or goal (in terms of features). Maintenance effort is what we would evaluate instead for every new feature added.

Like, if we want to show aggregate values like number of child processes running for a tree, it won't fit into any existing column. Correct me if you find I'm missing anything.

Do you mean the number of child process in the Running (R) state, or the number of child processes that have started and not dead? The latter includes processes in the Sleep (S) state and I/O waiting (D) state. You know, processes you could see in htop are usually in S state -- only a few of them would be R and consume CPU time.

An idea to explore may be implementing "multi-mode" columns

"multi-mode" columns also sound like a great idea. If we're having that, then we can also allow the user to add duplicate columns (same columns with different modes) to his screen.

I will let @BenBE correct me if I get this wrong, but my vision is that the columns' mode would be global, and you won't see per-process CPU% and aggregated CPU% side by side. Only one will be displayed at a time, but a hotkey would be available to quickly switch between modes -- that is, between per-process data and aggregated data.

@BenBE
Copy link
Member

BenBE commented Nov 27, 2023

@80avin:

I have a better vision for this: if the aggregated data for a process group or session is important and worth displaying separately from the parent process node, I would introduce separate rows for displaying the aggregated info. Such rows would be colored differently from the processes and threads (for a better visual distinction).

I think the idea needs to be refined more to be convincing. Not sure if it is only me but when I try to evaluate this idea based on the value it adds relative to the development effort, I don't find it much pleasing. Like, if we want to show aggregate values like number of child processes running for a tree, it won't fit into any existing column. Correct me if you find I'm missing anything.

Currently there's no real column for this. If you stretch definitions you could re-use the # of LWP column for this.

An idea to explore may be implementing "multi-mode" columns

"multi-mode" columns also sound like a great idea. If we're having that, then we can also allow the user to add duplicate columns (same columns with different modes) to his screen. But another perspective on the multi-mode column is that

  1. They will only reduce some clutter from Screen settings, as "Available Columns" will have one entry for process CPU & tree CPU. Otherwise, they'll be working the same as current PR from the user's perspective. Is this difference enough of value addition that we implement multi-mode feature for columns ?
  2. Different modes will still require different column names, so this "multi-mode" is just a way to pack multiple column definitions into one in the Settings only.

ACK.

@Explorer09:

I think the idea needs to be refined more to be convincing. Not sure if it is only me but when I try to evaluate this idea based on the value it adds relative to the development effort, I don't find it much pleasing.

Well, the development effort is not something I would evaluate as htop is an open source project without any definite deadline or goal (in terms of features). Maintenance effort is what we would evaluate instead for every new feature added.

Correct. And one of the reasons #303 didn't get anywhere yet is the vague definition for what constitutes an application and what not. But that's something to discuss over there.

Like, if we want to show aggregate values like number of child processes running for a tree, it won't fit into any existing column. Correct me if you find I'm missing anything.

Do you mean the number of child process in the Running (R) state, or the number of child processes that have started and not dead? The latter includes processes in the Sleep (S) state and I/O waiting (D) state. You know, processes you could see in htop are usually in S state -- only a few of them would be R and consume CPU time.

That would be either multiple columns, or with multi-mode columns this could be done as one mode:

  • All processes in subtree
  • All active
  • All pending/ready
  • All idle

An idea to explore may be implementing "multi-mode" columns

"multi-mode" columns also sound like a great idea. If we're having that, then we can also allow the user to add duplicate columns (same columns with different modes) to his screen.

I will let @BenBE correct me if I get this wrong, but my vision is that the columns' mode would be global, and you won't see per-process CPU% and aggregated CPU% side by side. Only one will be displayed at a time, but a hotkey would be available to quickly switch between modes -- that is, between per-process data and aggregated data.

Both a "global column mode" and a "per-column mode" are possible options. It's basically a question of which direction we decide to go with this. But I'm more in the "per-column mode" camp with that idea, as that is the most flexible and is the closest to what this PR already tries to do.

That being said: We should keep discussions for this PR on topic and split the "multi-mode columns" and "row grouping" (e.g. applications) to separate discussions to keep this PR manageable. Those topics would need to be addressed in separate PRs anyway.

Finally, while from a technical PoV this PR LGTM (sans notes) I'm not completely happy with the overall feature (despite generally being in favour for it). The issue I think we need to address before continuing is the general discussion about whether we'd potentially want to introduce quite a few (similar) such columns, or if there's a better way to go ahead with this.

@BenBE BenBE added the needs-discussion 🤔 Changes need to be discussed and require consent label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show total resources of multi process applications
3 participants